[WIP] feat(rest): allow bypassing http response writing and custom content type #1753

raymondfeng · 2018-09-24T17:13:37Z

We discover an issue to return html from a controller:
See loopbackio/loopback4-example-shopping#16

Checklist

npm test passes on your machine
New tests added or existing tests modified to cover all changes
Code conforms with the style guide
API Documentation in code was updated
Documentation in /docs/site was updated
Affected artifact templates in packages/cli were updated
Affected example projects in examples/* were updated

virkt25

LGTM 🚀

raymondfeng · 2018-09-24T20:45:19Z

@virkt25 I refined the PR to check if a controller method returns the response object itself as the flag to skip writing. PTAL.

virkt25

I like the new bypass much better!! Should we add a section in the README showing how to just set the content type and let the writer write the response

virkt25 · 2018-09-24T23:29:14Z

docs/site/Controllers.md

@@ -214,6 +214,41 @@ export class HelloController {
 - `@param.query.number` specifies in the spec being generated that the route
  takes a parameter via query which will be a number.

+## Custom Response Writing
+
+By default, LoopBack serialize the return value from the controller methods into


LoopBack will serialize
OR
LoopBack serializes the ...

virkt25 · 2018-09-24T23:33:29Z

packages/rest/src/writer.ts

    response.end();
    return;
  }

-  if (result instanceof Readable || typeof result.pipe === 'function') {
-    response.setHeader('Content-Type', 'application/octet-stream');
+  function setContentType(defaultType: string = 'application/json') {


the variable name should be changed to type as it can be overridden.

…type We discover an issue to return html from a controller: See loopbackio/loopback4-example-shopping#16

bajtos · 2018-09-25T09:22:00Z

I am rather unhappy about the design changes proposed in this pull request.

So far, the framework was offering pretty strong guarantees to LoopBack users: every HTTP response was produced either by send or reject. If an application or an extension wants to intercept or modify all responses, and it provides custom send & reject actions, then it can be pretty certain it has all cases covered.

The solution proposed by this pull request, where the controller sends the response directly via Express response API, is throwing all guarantees out.

I understand we need a quick workaround to enable our shopping-cart example to serve a home page (loopbackio/loopback4-example-shopping#16) and I am fine to add an undocumented fix as a short-term workaround.

I would personally use the following check to determine whether the response have been already produced:

The controller method returned undefined AND
The response headers have been already sent (see Node.js API response.headersSent)

What's wrong with letting the controller method return response? I don't like the fact that a controller method can return the response and yet don't send any data back. We will end up with a client connection hanging forever in such case.

For a longer term solution that will be documented, I'd like us to use the approach described in #436, #436 (comment) and #788 (comment), where the controller method can return an object describing the response to be produced, e.g. {statusCode: 201, headers: {/*...*/}, body: data}.

To move this pull request forward, I am asking for the following changes:

Reduce the amount of functionality changes. Just add a check to send action to return immediately when the response have been already send by the controller.
Either keep this feature undocumented, or else make it very clear that this feature is a short-term hack.

bajtos · 2018-09-25T09:25:12Z

packages/rest/src/types.ts

+export type Send = (
+  response: Response,
+  result: OperationRetval,
+  contentType?: string,


I don't understand the purpose of this new parameter. I don't see it used anywhere in production code, only in unit tests.

send is typically invoked from a Sequence. There is usually the same sequence of actions implemented for all endpoints. IMO, the decision which content-type to send back should be made in individual controller methods (or routes), not in the Sequence.

I am proposing to revert this change.

bajtos · 2018-09-25T09:25:23Z

packages/rest/src/writer.ts

 */
 export function writeResultToResponse(
  // not needed and responsibility should be in the sequence.send
  response: Response,
  // result returned back from invoking controller method
  result: OperationRetval,
+  // content type for the response
+  contentType?: string,


Similarly here, let's revert this change please.

bajtos · 2018-09-25T09:40:06Z

packages/rest/src/writer.ts

-  if (result instanceof Readable || typeof result.pipe === 'function') {
-    response.setHeader('Content-Type', 'application/octet-stream');
+  function setContentType(defaultType: string = 'application/json') {
+    if (response.getHeader('Content-Type') == null) {


Personally, I prefer to revert this change.

Ideally, there should be only two ways how a content-type is determined:

The controller method/route returns data to be serialized to the response. The content-type is determined by the framework depending on the type of data (object vs. string vs. stream or buffer), request headers like Accepts and endpoint OpenAPI spec. Right now, we have a very limited version of this algorithm implemented in the current writeResultToResponse code (objects are sent as JSON, streams as binary, strings as text).

The controller wants to set an explicit content type, status code or change any other aspect of the HTTP response. It returns an object describing response properties (status code, headers, body) and the send action applies these instructions to the response object. This will be enabled by Configurable response types/formats #436 in the future.

The change proposed here adds a third mode that makes it difficult to reason about the HTTP responses when reading code, because different aspects of the response (content type vs. actual response body) are produced by different pieces of code.

The controller (or a sequence action in general) explicitly sets the content type, but it still expects the framework to convert the value returned by the controller method (or route handler) into HTTP response body.

There is of course also the fourth mode being added here, I think it should be the only scope of this pull request. It goes against the two modes I described above, but I ok with it as a temporary workaround:

The controller (or a sequence action in general) takes full control of the response sent: it sets the status code & headers and writes the response body. The send action becomes a no-op.

bajtos · 2018-09-25T09:42:49Z

packages/rest/src/writer.ts

+
+  if (
+    result instanceof Readable ||
+    typeof (result && result.pipe) === 'function'


https://loopback.io/doc/en/contrib/style-guide.html#indentation-of-multi-line-expressions-in-if

const isStream = result instanceof Readable || typeof (result && result.pipe) === 'function'; if (isStream) { // ... }

raymondfeng · 2018-09-25T15:28:21Z

@bajtos Thank you for the feedback. A few clarifications:

I introduced the extra contentType argument in the hope of being able to infer the value from the response object. But that seems to be a bit involved to implement at the moment. As you mentioned, the final content type can be a result of content negotiation.
I was thinking about to introduce a new HttpResponse return type with the fluent API so that a controller method can return a fully-populated response so that our writer can honor it. But I realized most of the APIs are already available on Response and I didn't want to reinvent the wheel.
There is a pull and push to write the http response.

pull: The writer pulls data from the controller method return value and serialize them into the response
push: The controller method serializes data into the response

I see use cases for both modes and would love to allow both flavors. We can treat the push mode as the controller method has its own writer.

The issues above need to be addressed in the long run. To enable our shopping example, I'll create a different (much simpler) PR to allow:

The controller (or a sequence action in general) takes full control of the response sent: it sets the status code & headers and writes the response body. The send action becomes a no-op.

virkt25 · 2018-09-25T15:43:41Z

Isn't it better to allow the controller to just set the response type / response header? ... How much work is it for us to implement pulling the controller metadata to see what the response header should be and set it based on that?

raymondfeng · 2018-09-26T17:53:35Z

Close it to favor #1760

raymondfeng requested a review from bajtos as a code owner September 24, 2018 17:13

raymondfeng force-pushed the improve-http-response-writing branch from 7af3258 to d986a61 Compare September 24, 2018 17:34

virkt25 approved these changes Sep 24, 2018

View reviewed changes

raymondfeng force-pushed the improve-http-response-writing branch 4 times, most recently from df2a5fa to 21c4f41 Compare September 24, 2018 20:44

raymondfeng force-pushed the improve-http-response-writing branch from 21c4f41 to b4808b0 Compare September 24, 2018 21:13

virkt25 reviewed Sep 24, 2018

View reviewed changes

feat(rest): allow bypassing http response writing and custom content …

deb2f05

…type We discover an issue to return html from a controller: See loopbackio/loopback4-example-shopping#16

raymondfeng force-pushed the improve-http-response-writing branch from b4808b0 to deb2f05 Compare September 25, 2018 02:03

bajtos suggested changes Sep 25, 2018

View reviewed changes

raymondfeng added the post-GA label Sep 25, 2018

raymondfeng changed the title ~~feat(rest): allow bypassing http response writing and custom content type~~ [WIP] feat(rest): allow bypassing http response writing and custom content type Sep 25, 2018

raymondfeng mentioned this pull request Sep 25, 2018

feat(rest): allow controller methods to handle response writing #1760

Merged

7 tasks

raymondfeng closed this Sep 26, 2018

bajtos deleted the improve-http-response-writing branch November 20, 2018 13:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] feat(rest): allow bypassing http response writing and custom content type #1753

[WIP] feat(rest): allow bypassing http response writing and custom content type #1753

raymondfeng commented Sep 24, 2018 •

edited

Loading

virkt25 left a comment

raymondfeng commented Sep 24, 2018

virkt25 left a comment

virkt25 Sep 24, 2018

virkt25 Sep 24, 2018

bajtos commented Sep 25, 2018

bajtos Sep 25, 2018

bajtos Sep 25, 2018

bajtos Sep 25, 2018 •

edited

Loading

bajtos Sep 25, 2018

raymondfeng commented Sep 25, 2018 •

edited

Loading

virkt25 commented Sep 25, 2018

raymondfeng commented Sep 26, 2018

[WIP] feat(rest): allow bypassing http response writing and custom content type #1753

[WIP] feat(rest): allow bypassing http response writing and custom content type #1753

Conversation

raymondfeng commented Sep 24, 2018 • edited Loading

Checklist

virkt25 left a comment

Choose a reason for hiding this comment

raymondfeng commented Sep 24, 2018

virkt25 left a comment

Choose a reason for hiding this comment

virkt25 Sep 24, 2018

Choose a reason for hiding this comment

virkt25 Sep 24, 2018

Choose a reason for hiding this comment

bajtos commented Sep 25, 2018

bajtos Sep 25, 2018

Choose a reason for hiding this comment

bajtos Sep 25, 2018

Choose a reason for hiding this comment

bajtos Sep 25, 2018 • edited Loading

Choose a reason for hiding this comment

bajtos Sep 25, 2018

Choose a reason for hiding this comment

raymondfeng commented Sep 25, 2018 • edited Loading

virkt25 commented Sep 25, 2018

raymondfeng commented Sep 26, 2018

raymondfeng commented Sep 24, 2018 •

edited

Loading

bajtos Sep 25, 2018 •

edited

Loading

raymondfeng commented Sep 25, 2018 •

edited

Loading